-
Notifications
You must be signed in to change notification settings - Fork 299
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add explicit tie-breaker in pending balance deposits sort #8772
Conversation
I'm not sure this won't trigger another sorting round. |
I think this doesn't trigger an additional round, should be indistinguishable from a perf perspective IMO. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Hmm I don't think so. I made this simple test. See that @Test
void tieBreakerIsNotCalled() {
record Item(String name, int foo, int bar) {}
final List<Item> items = new ArrayList<>();
items.add(new Item("a", 10, 100));
items.add(new Item("b", 20, 200));
items.add(new Item("c", 10, 150));
final var sorted = IntStream.range(0, items.size()).boxed().sorted(
Comparator.comparing(
(Integer index) -> {
System.out.println("Comparing " + items.get(index).name + ".foo: " + items.get(index).foo);
return items.get(index).foo;
}).thenComparing((Integer index) -> {
System.out.println("Comparing " + items.get(index).name + ".bar: " + items.get(index).bar);
assertThat(items.get(index).name).isNotEqualTo("b");
return items.get(index).bar;
}));
sorted.forEach(System.out::println);
}
Yes, this is good idea. But it will need the |
About the parallelism argument, we do parallel stream only in one case, (IIRC to go through all validators). To make it profitable you need to have a lot of elements to stream (or a complex processing over each element), otherwise you're just adding thread coordination overhead. BTW |
PR Description
Out of caution, this PR adds a
thenComparing(index)
call when sorting validators for pending balance deposits. See a screenshot of the relevant spec below. Because we're iterating from 0-len(vals), the current implementation should be fine but if this doesn't affect performance (which it shouldn't) I see no harm in adding this.Documentation
doc-change-required
label to this PR if updates are required.Changelog